feat: Add tower tracing #431
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #431 +/- ##
=======================================
+ Coverage 54.4% 54.9% +0.4%
=======================================
Files 71 71
Lines 11892 12061 +169
=======================================
+ Hits 6476 6624 +148
- Misses 5416 5437 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…erfer with each-other
|
while working on this, I started to wonder how the overall strategy for this crate could look like, and I am wondering if my PR is maybe not going far enough. For the future we have multiple use cases for a (collection of?) tower layer(s). So far I see at least 4:
As it stands now, it provides HTTP server support, for that it might make sense to also rename the structs accordingly to It might even be an idea to provide some form of auto-detecting abstraction on top which figures out if it's in a server/client context or gRPC/HTTP exchange. I am not sure how realistic that is though, as I haven't looked into the details yet. |
| label_superset.extend(custom_response_attributes.clone()); | ||
|
|
||
| // Update span | ||
| let span = this.future_state.otel_context.span(); |
There was a problem hiding this comment.
if we activate the OTel Context in the request, we should be able to retrieve it from Context::Current instead of having to store it ourselves.
There was a problem hiding this comment.
That's not possible, I believe, or I am not sure how to solve it. The guard is a raw pointer (*const()) which means it is !Send but we have to store it for the response future to not drop it and lose the guard and for that it must be Send.
There was a problem hiding this comment.
I rewrote the response future to be able to attach the context to it, which solves this problem. Please have another look!
opentelemetry-instrumentation-tower/examples/axum-http-service/src/main.rs
Outdated
Show resolved
Hide resolved
opentelemetry-instrumentation-tower/examples/axum-http-service/src/main.rs
Outdated
Show resolved
Hide resolved
Sincere apologies for delay in getting back to reviewing. I left some comments in the PR. Its mostly working - the key part I want to change are
|
jan-xyz
force-pushed
the
add_tower_tracing
branch
from
5974fed to
eaf22d3
Compare
2 participants
Changes
This adds tracing support for the tower layer. It was addressed briefly with @cijothomas on the CNCF slack, but we didn't discuss any details of the implementation. I took the liberty for now to take 2 major design decisions:
General vs. Specialized tower layer: I chose to pick a general layer that does metrics and tracing together with a no-op tracer in case someone doesn't want to do tracing with this layer. I assume that long term people/teams will fully switch to OpenTelemetry for Metrics and Tracing and instead of specifying two layers which each inspect the request and response, it makes more sense to do this in one layer.
Backwards compatibility: I tried to leave the original types in place which have the "Metric" implementation in the name. This is maybe a more smooth migration? I am not sure how helpful that actually is, and maybe we should just break compatibility in case we want to go with a unified layer.
Note: It currently does not include context extraction.
As a follow up I would also like to provide a way to use this as a client layer, where the semantics change a bit.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes